Skip to content

refactor: add serialized class defs newtypes#3340

Merged
CHr15F0x merged 20 commits intomainfrom
chris/serialized-class-defs-newtypes
Apr 20, 2026
Merged

refactor: add serialized class defs newtypes#3340
CHr15F0x merged 20 commits intomainfrom
chris/serialized-class-defs-newtypes

Conversation

@CHr15F0x
Copy link
Copy Markdown
Contributor

@CHr15F0x CHr15F0x commented Apr 15, 2026

Fixes: #3326

Rationale: we have 4 distinct types which are all passed around as Vec<u8>. Just until recently this seemed good enough (though absolutely against static typing and a "everything is a type approach" that leverages the compiler), until it didn't. I spent a good couple of days trying to find a regression that I introduced, which was a simple copy-pasta caused by using a Vec<u8> Sierra where Casm was actually expected. That was enough.


The PR introduces 4 types:

  • SerializedSierraDefinition
  • SerializedCasmDefinition
  • SerializedCairoDefinition
  • SerializedOpaqueClassDefinition, where the internal Vec<u8> is either sierra or cairo, but we don't really care because it's all passed around to the end consumer without the need to figure out which variant it really is; this is also how classes are stored and served via the RPC
  • SerializedClassDefinition, which is the opposite of the above - it's an enum, which distinguishes between the two variants

I tried to make the changes transparent and not modify the semantics of the APIs, with one notable exception: compute_class_hash, where it actually made sense to consume the buffer and reinterpret it as either of the variants in a safe manner, so that download_class itself does not have to do the reinterpreting in an unsafe manner.

I left all the fixtures as is to keep the delta reasonably small (which it already isn't).

@CHr15F0x CHr15F0x force-pushed the chris/serialized-class-defs-newtypes branch 5 times, most recently from 5646390 to 2c0a166 Compare April 16, 2026 08:45
@CHr15F0x CHr15F0x marked this pull request as ready for review April 16, 2026 09:01
@CHr15F0x CHr15F0x requested a review from a team as a code owner April 16, 2026 09:01
@CHr15F0x CHr15F0x changed the title WIP refactor: add serialized class defs newtypes refactor: add serialized class defs newtypes Apr 16, 2026
Comment thread crates/storage/src/fake.rs Outdated
@CHr15F0x CHr15F0x force-pushed the chris/serialized-class-defs-newtypes branch from defb65a to 3de8656 Compare April 16, 2026 15:00
Copy link
Copy Markdown
Contributor

@zvolin zvolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactor 💪

Comment thread crates/class-hash/src/lib.rs Outdated
Comment thread crates/class-hash/src/lib.rs Outdated
Comment thread crates/common/src/casm_class.rs Outdated
Comment thread crates/compiler/src/lib.rs
Comment thread crates/executor/src/state_reader.rs Outdated
Comment thread crates/rpc/src/method/add_declare_transaction.rs Outdated
Comment thread crates/rpc/src/executor.rs Outdated
Comment thread crates/common/src/class_definition.rs
Comment thread crates/common/src/class_definition.rs
Copy link
Copy Markdown
Contributor

@t00ts t00ts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for doing this 💪

Comment thread crates/executor/src/state_reader.rs Outdated
Comment thread crates/pathfinder/src/p2p_network/sync/sync_handlers.rs Outdated
@CHr15F0x CHr15F0x force-pushed the chris/serialized-class-defs-newtypes branch from 2d0b1a8 to 137f37d Compare April 20, 2026 07:24
@CHr15F0x CHr15F0x merged commit d074cc9 into main Apr 20, 2026
11 checks passed
@CHr15F0x CHr15F0x deleted the chris/serialized-class-defs-newtypes branch April 20, 2026 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: Introduce separate types for serialized casm and sierra definitions

3 participants